Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ORCID login when no family name was given #914

Merged
merged 2 commits into from
May 16, 2024
Merged

Fix ORCID login when no family name was given #914

merged 2 commits into from
May 16, 2024

Conversation

nikoder
Copy link
Contributor

@nikoder nikoder commented May 15, 2024

Handle the case where the user has not registered a family-name and ORCID returns None.

Proposed changes

This addresses python-social-auth/social-app-django#355 and #602 (which it appears to duplicate). Full credit to paloha, who very clearly explained the issue and proposed the simple fix included here.

As a brief summary: ORCID does not require users to enter a family name and returns a family-name key with value None in this case. The code expects that key to either be absent, or contain a dictionary. This leads to an AttributeError after successful login by a user who has not provided a family name to ORCID.

I considered adding a test case to cover this scenario, but it looks like the testing infrastructure is only set up for a single possible user_data_body? Please let me know if I have missed an easy and obvious way to add a test for an additional case of returned data. The change does resolve the issue in my manual testing.

Types of changes

Please check the type of change your PR introduces:

  • Release (new release request)
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Code style update (PEP8, lint, formatting, renaming, etc)
  • Refactoring (no functional changes, no api changes)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Build related changes (build process, tests runner, etc)
  • Other (please describe):

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask. We're here to
help! This is simply a reminder of what we are going to look for before merging
your code.

  • Lint and unit tests pass locally with my changes - caveat: 6 of 587 discovered tests fail independently of these changes in my environment but they do not appear to be covering the changed code (they are related to SAML)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added documentation to https://github.com/python-social-auth/social-docs

Other information

Any other information that is important to this PR such as screenshots of how
the component looks before and after the change.

Handle the case where the user has not registered a `family-name` and
ORCID returns `None`.
@nikoder nikoder changed the title Fix social-app-django/issues/355 Fix ORCID login when no family name was given May 15, 2024
@nijel nijel merged commit 95316b0 into python-social-auth:master May 16, 2024
7 checks passed
@nijel
Copy link
Member

nijel commented May 16, 2024

Merged, thanks for your contribution!

@nijel nijel self-assigned this May 16, 2024
@nikoder
Copy link
Contributor Author

nikoder commented May 16, 2024

My pleasure! Thanks for your great work on this awesome project :) !

@nikoder nikoder deleted the fix/exception-when-missing-orcid-family-name branch May 16, 2024 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants